Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tail config #484

Closed
wants to merge 25 commits into from
Closed

Add tail config #484

wants to merge 25 commits into from

Conversation

sjoshid
Copy link

@sjoshid sjoshid commented Nov 23, 2019

Closes xi-editor/xi-editor#922.
This PR adds a "Tail File" option under Debug menu. This option can be enabled/disabled per file.

TODO: Gray out this option if the file doesnt exist. No point in tailing a file that doesnt exist.

Review Checklist

  • I have responded to reviews and made changes where appropriate.
  • I have tested the code
  • I have updated comments / documentation related to the changes I made.
  • I have rebased my PR branch onto xi-mac/master.

Copy link
Member

@nangtrongvuon nangtrongvuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good at a first glance, although there are a few things I'd like to note:

  • I highly recommend using the "Automatically trim trailing whitespace" feature in Xcode. The diff has a lot of unrelated whitespace changes, which can make it hard for reviewers to focus on the parts that matter.
  • If I toggle tail on a file and I open another file, this causes a crash.

I do think tailing is a super cool feature that I'd like to have, since one of the things I commonly use xi for is to read logs. Hoping to see the next iterations of this feature!

@sjoshid
Copy link
Author

sjoshid commented Dec 4, 2019

I highly recommend using the "Automatically trim trailing whitespace" feature in Xcode. The diff has a lot of unrelated whitespace changes, which can make it hard for reviewers to focus on the parts that matter.

When I started working on this we were targeting this to be like tail command. But Im open for automatic trimming. Will look in to this.

If I toggle tail on a file and I open another file, this causes a crash.

Hmm. I cannot replicate this issue. Can you please provide more details? Keep in mind if you try to tail a file that doesnt exists, it does crash. (See TODO in description) But not sure if this is what you are trying to do.

I do think tailing is a super cool feature that I'd like to have, since one of the things I commonly use xi for is to read logs

Same here. There arent any editors out there that do this efficiently, and frankly the way tail does it.
Thanks for reviewing this. I'll keep updating this PR. Hopped on just to give an update and try the crash use case.

@nangtrongvuon
Copy link
Member

I highly recommend using the "Automatically trim trailing whitespace" feature in Xcode. The diff has a lot of unrelated whitespace changes, which can make it hard for reviewers to focus on the parts that matter.

When I started working on this we were targeting this to be like tail command. But Im open for automatic trimming. Will look in to this.

Oh, I don't mean that the tail feature should trim whitespaces, but I was assuming you made this pull request in Xcode. I mean that you should turn on that setting in Xcode, so your diff doesn't have too many whitespace changes.

Other than that, looking forward to your tail work!

@sjoshid
Copy link
Author

sjoshid commented Dec 4, 2019

haha. silly me.
Will get to this as soon as I get some time.

Thanks.

@nangtrongvuon
Copy link
Member

Heya, I took another look at this, and the changes look good! Here are some of my comments on this PR before it can be merged:

  • Current CI is saying the PR doesn't build properly because core doesn't build properly, you might want to check the corresponding core commit.
  • The PR's commit log is a bit messy. I suggest just extracting the current changes here and making just one commit that's based off the current upstream master tip.
  • This is a just a big nit, but the whitespaces are still in the changelist. I think if you follow the "making a new commit advice", that should solve the whitespaces problem with the Xcode trim whitespace setting turned on as well.

@sjoshid
Copy link
Author

sjoshid commented Dec 15, 2019

Current CI is saying the PR doesn't build properly because core doesn't build properly, you might want to check the corresponding core commit.

Yeah. Core is build successfully yesterday. Is there a way to force build?

The PR's commit log is a bit messy. I suggest just extracting the current changes here and making just one commit that's based off the current upstream master tip.

I work on multiple machines so I commit everything I can which adds to the messyness. I will squash commits.

This is a just a big nit, but the whitespaces are still in the changelist. I think if you follow the "making a new commit advice", that should solve the whitespaces problem with the Xcode trim whitespace setting turned on as well.

I have the trim whitespace setting turned on for sure. I'll try the advice thing when I squash commits.

@nangtrongvuon
Copy link
Member

You can force a rebuild simply by force pushing to this branch again, that's what I do all the time too. :P

@sjoshid
Copy link
Author

sjoshid commented Dec 15, 2019

@nangtrongvuon
Two things

  1. I squashed commits. Dont think there is a way to reflect that in this PR. I still see it says there are 23 commits but in fact there are only 4. This could be because I created this branch from my forked master where I hadnt squashed commits. :(. so maybe I can create a new PR from the new branch again? If I do, it'll show only 4 commits.
  2. The build keeps failing. Any idea why? Here's my core PR Tail toggle v2 xi-editor#1241.

This file needs to be reverted after that core's branch is merged but before this branch is merged here. 
Without this change, Xi mac will not build.
@sjoshid
Copy link
Author

sjoshid commented Dec 17, 2019

Im going to close this PR. There are too many unnecessary commits. I'll open a new PR with required changes.

@sjoshid sjoshid closed this Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tail toggle?
2 participants